Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DeleteComment conflict resolver #50919

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

DeleteComment conflict resolver #50919

wants to merge 9 commits into from
+784 −19

Conversation

gedu
Copy link
Contributor

@gedu gedu commented Oct 16, 2024

Details

Any request with command AddXXX that I find, I will try to not perform a request, given that the backend doesn't have information about it.
If there is no AddXXX or it is already processing, I will delete any actions between that processing/existing comment and the DeleteComment. The server just need to delete it and not perform any kind of update like (UpdateComment, AddEmojiReaction, etc)
There is a special case, when the comment will be use to create a thread (an OpenReport is executed) in that case, the safer solution is to send all the requests as planned instead of deleting, given that DeleteComment is by reportActionsID and the logic could be complicated when multiple random actions happens in a thread.

Fixed Issues

$ #50074
PROPOSAL: -

Tests

DeleteComment request:

  • Send a message (wait for this message to success)

  • Go to Offline

  • Edit the message

  • Add emoji reaction

  • Remove emoji reaction

  • Delete the message

  • Go to online

  • Just the DeleteComment request should be send

  • Verify that no errors appear in the JS console

Offline tests

No requests:

  • Go to offline
  • send a message
  • edit the message
  • add a emoji reaction
  • delete the message
  • Go to online
  • No requests should be sent

Threads

  • Go to offline
  • Send a message
  • create a thread
  • delete that message
  • Go to online
  • All the request must be sent

QA Steps

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
    • MacOS: Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
deleteComment_chrome.mp4
MacOS: Desktop

@gedu gedu requested a review from a team as a code owner October 16, 2024 15:46
@melvin-bot melvin-bot bot requested review from shubham1206agra and removed request for a team October 16, 2024 15:46
@melvin-bot Melvin Bot
Copy link

melvin-bot bot commented Oct 16, 2024

@shubham1206agra Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@gedu gedu mentioned this pull request Oct 16, 2024
Open
@gedu gedu force-pushed the gedu/delete_comment_conflict_resolver branch from cc4fb33 to b26e6cd Compare October 16, 2024 18:35
@mountiny
Copy link
Contributor

@shubham1206agra let us know what is your eta for this review? thanks!

@gedu
Copy link
Contributor Author

gedu commented Oct 21, 2024

@shubham1206agra any news?

@gedu
Merge branch 'main' into gedu/delete_comment_conflict_resolver
aa0b105
@gedu
Merge branch 'main' into gedu/delete_comment_conflict_resolver
06cdcf9 
# Conflicts:
#	src/libs/actions/Report.ts
#	src/libs/actions/RequestConflictUtils.ts
#	tests/actions/ReportTest.ts
#	tests/unit/RequestConflictUtilsTest.ts
@mountiny
Copy link
Contributor

I think Shubham is going to be busy with some other projects now so in sake of focus I will try to reassign

@mountiny mountiny requested review from c3024 and removed request for shubham1206agra October 22, 2024 02:34
@c3024
Copy link
Contributor

c3024 commented Oct 22, 2024 •
edited
Loading

I found a small problem. Until we process the response, we do not remove the request from PersistentRequests. So, if a request is made, but before the response is received, the client can go offline. So, the request is not removed from PersistentRequests.

So,

  1. Client makes an AddXXX request.
  2. But, before receiving the response for this request, it goes offline.
  3. Now, we do a DeleteXXX action. This DeleteXXX request removes the AddXXX and itself from PersistedRequests.
  4. But, backend receives the request made in step 1 and includes the successful changes in the ReconnectApp when the client goes back online.
  5. But, the DeleteXXX request is lost so request for deleting the comment is not sent to backend.

Here is a video with 8kbps download speed

deleteRequestLost.mp4

I think we should consider not removing a DeleteXXX request if a corresponding Add/Update requests were already tried by the client.

@gedu
Copy link
Contributor Author

gedu commented Oct 23, 2024

Looking into that, seems that when you go offline a multiple request is added, ReconnectApp, and the conflict resolution just get one item:
Screenshot 2024-10-23 at 09 16 44
Just one ReconnectApp which is fine, so it replace it
But when the DeleteCommand is push, the conflict resolution gets the queue with the AddComment in it
Screenshot 2024-10-23 at 09 18 49
(as you can see happens after the reconnect so it shouldn't be there, that's why DeleteComment delete itself)

@gedu
Copy link
Contributor Author

gedu commented Oct 23, 2024

@mountiny @c3024 this is a bug that is happening on main too, I just checked and it sends duplicated AddComment requests.
Screenshot 2024-10-23 at 12 00 52

My code is preventing the duplicated request to be sent, so It can't delete the original because as stated the DeleteComment deletes itself.

Seems something really weird, seems that the ReportActionCompose is adding back the last text, and submitting it.
What should I do? try to fix it or we will create a new issue for this?

@c3024
Copy link
Contributor

c3024 commented Oct 23, 2024

I don't think calling AddComment again is a bug.

When a client makes a write request, unless it receives a response it cannot know that the request is successful.

In this case, the first AddComment request succesfully reached backend. But, sometimes the request can also not reach backend. So, it should not be removed from PersistedRequests just after client making the request but should be removed only after receiving the response here.

App/src/libs/Network/SequentialQueue.ts

Line 99 in f251dbc

PersistedRequests.remove(requestToProcess);

When the client fails to stay online till the response returns, this ongoing request, AddComment here, is added back to PersistedRequests.

App/src/libs/Network/SequentialQueue.ts

Line 111 in f251dbc

PersistedRequests.rollbackOngoingRequest();

If client sends the AddComment again, I think backend has some way to understand that this is a duplicate request that was already processed. It must not be doing anything.

Also, when we go online, ReconnectApp sends back the latest update which in this case includes the first successful AddComment also, so the message appears again on the screen.

So, I think if we can know that client tried an AddXXX request somehow, we should not remove the DeleteXXX request corresponding to the AddXXX action in this case because AddXXX might have been successful on the backend. So, we should not delete the DeleteXXX request and should send this request to backend.

@gedu
Copy link
Contributor Author

gedu commented Oct 23, 2024

Ok, will take a look at that case

@gedu
Copy link
Contributor Author

gedu commented Oct 23, 2024

ok, I will add inside of RequestConflictResolver type a isRollbacked field. I'm testing and trying to create some tests

@gedu
Add new field to know if a request was rollbacked
c93b9b4
@gedu
Merge branch 'main' into gedu/delete_comment_conflict_resolver
ca92e8b 
# Conflicts:
#	src/libs/actions/Report.ts
#	src/libs/actions/RequestConflictUtils.ts
#	tests/actions/ReportTest.ts
#	tests/unit/RequestConflictUtilsTest.ts
@gedu
Copy link
Contributor Author

gedu commented Oct 24, 2024

@c3024 PR updated

@gedu gedu mentioned this pull request Oct 24, 2024
Open
50 tasks
@c3024
Copy link
Contributor

c3024 commented Oct 24, 2024

Offline behaviour with "Force offline" is different for Threads Test.

forceOffline.mp4

This should not be a problem for production because that option does not exist in production but QA will see a different result here for the test on staging.

@c3024
Copy link
Contributor

c3024 commented Oct 25, 2024

@gedu, can you please check the above comment? I wonder why "Force offline" is behaving differently from setting offline in Dev Tools or disconnecting network on device settings level.

@gedu
Copy link
Contributor Author

gedu commented Oct 25, 2024

@c3024 yes, taking a look now

@gedu
Copy link
Contributor Author

gedu commented Oct 25, 2024

@c3024 Fix it and added a test to cover that case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@c3024 c3024 Awaiting requested review from c3024

At least 1 approving review is required to merge this pull request.

Assignees

@gedu gedu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gedu @mountiny @c3024